drop duplicate listing coordinates#45
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Updates to Preview Branch (dnywh/drop-duplicate-listing-coordinates) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
There was a problem hiding this comment.
Pull request overview
Removes duplicated latitude/longitude storage on public.listings, standardizing on PostGIS location as the single coordinate source, and updates DB views/RPC + frontend flows to consume a computed coordinates object.
Changes:
- Drops
latitude/longitudecolumns frompublic.listingsand rebuilds related views +listings_in_view(...)RPC to exposecoordinates(jsonb). - Updates map and listing read/edit UI paths to read from
listing.coordinatesand write onlylocation. - Updates seed data to insert/update listings without latitude/longitude columns.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/seed.sql | Removes latitude/longitude from seeded listing inserts/updates to match the new schema. |
| supabase/migrations/20260419093000_drop_duplicate_listing_coordinates.sql | Enforces coordinate consistency, drops duplicate columns, rebuilds views and viewport RPC to return coordinates. |
| src/components/MapImmersive/MapImmersive.jsx | Switches map centering/markers to use listing.coordinates instead of flat lat/long. |
| src/components/ListingWrite/ListingWrite.tsx | Initializes/edit form coordinates from initialListing.coordinates and stops writing lat/long columns. |
| src/components/ListingRead/ListingRead.jsx | Updates map thumbnail + external map links to use listing.coordinates. |
| src/app/(forms)/profile/listings/[slug]/page.js | Fetches editable listings from listings_private_data (which now exposes coordinates). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| alter table public.listings | ||
| alter column location type extensions.geography(Point, 4326) | ||
| using extensions.st_setsrid(location::extensions.geometry, 4326)::extensions.geography(Point, 4326); | ||
|
|
||
| create index if not exists listings_location_geometry_index | ||
| on public.listings | ||
| using gist ((location::extensions.geometry)); | ||
|
|
There was a problem hiding this comment.
This migration adds a new GiST index on (location::geometry), but the baseline schema already has a GiST index on listings.location ("listings_geo_index"). Unless you still need both geography and geometry indexes for different query patterns, keeping both will add write amplification and extra storage. Consider dropping the unused one (or replacing the existing index) so only the index that matches your spatial operators remains.
| initialViewState={{ | ||
| longitude: | ||
| (hasValidCoordinates(selectedListing) | ||
| ? selectedListing.longitude | ||
| ? selectedListing.coordinates.longitude | ||
| : null) ?? | ||
| initialCoordinates?.longitude ?? | ||
| DEFAULT_COORDINATES.longitude, | ||
| latitude: | ||
| (hasValidCoordinates(selectedListing) | ||
| ? selectedListing.latitude | ||
| ? selectedListing.coordinates.latitude | ||
| : null) ?? |
There was a problem hiding this comment.
You introduced getListingCoordinates() to centralize coordinate access, but initialViewState still reads selectedListing.coordinates.* directly. For consistency (and to reduce the chance of a future shape change missing one call site), consider using getListingCoordinates(selectedListing) here as well (or destructure once and reuse).
6a9ffc4 to
ae1e8bd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If there's a selected listing in URL with valid coords, center on it | ||
| if (hasValidCoordinates(selectedListing)) { | ||
| const coordinates = getListingCoordinates(selectedListing); | ||
|
|
There was a problem hiding this comment.
The one-time useEffect that does initial positioning reads selectedListing/initialCoordinates, but it has an empty dependency array. If selectedListing is loaded asynchronously after mount (common when resolving the URL slug), this block won’t re-run, so the map may never center on the selected listing. Consider adding a dedicated useEffect that watches selectedListing?.coordinates (and a “has centered yet” ref/state) and calls flyTo once when the first valid selected listing becomes available, or move this behavior into the selectedListing-change path explicitly.
ae1e8bd to
0186e9e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| if ( | ||
| !isFirstLoad.current || | ||
| selectedListing || | ||
| !initialCoordinates || | ||
| !mapRef.current | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| isFirstLoad.current = false; | ||
| mapRef.current.flyTo({ | ||
| center: [initialCoordinates.longitude, initialCoordinates.latitude], | ||
| zoom: initialCoordinates.zoom, | ||
| duration: 0, | ||
| }); | ||
| }, [initialCoordinates, mapRef, selectedListing]); |
There was a problem hiding this comment.
isFirstLoad.current is only set to false when the "IP coordinates" flyTo runs. If the page loads with a selected listing (URL param) you never clear isFirstLoad, so later when the user clears selectedListing this effect can fire and unexpectedly recenter the map to initialCoordinates. Consider setting isFirstLoad.current = false unconditionally after the first mount/map load, and gating the initialCoordinates centering with a separate ref that only tracks whether the initialCoordinates centering has already run (rather than reusing isFirstLoad).
| useEffect(() => { | ||
| if ( | ||
| !mapRef.current || | ||
| !hasValidCoordinates(selectedListing) || | ||
| hasCenteredSelectedListingRef.current | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const coordinates = getListingCoordinates(selectedListing); | ||
|
|
||
| mapRef.current.flyTo({ | ||
| center: [coordinates.longitude, coordinates.latitude], | ||
| zoom: 12, | ||
| duration: 0, | ||
| }); | ||
| hasCenteredSelectedListingRef.current = true; | ||
| }, [ | ||
| mapRef, | ||
| selectedListing, | ||
| selectedListingCoordinates?.latitude, | ||
| selectedListingCoordinates?.longitude, | ||
| ]); |
There was a problem hiding this comment.
hasCenteredSelectedListingRef is never reset when selectedListing changes, so this effect will only auto-center for the first listing that becomes selected during the component’s lifetime. If the user navigates to a different listing via URL/history (or selectedListing changes to a different id), the map won’t recenter. Consider tracking the last centered listing id/slug and resetting the ref when that identifier changes, or make the guard conditional on matching the currently selected listing.
0186e9e to
2c1bd13
Compare
2c1bd13 to
cc5400b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| if ( | ||
| hasAppliedInitialPositionRef.current || | ||
| selectedListing || |
There was a problem hiding this comment.
The initial-positioning effect returns early whenever selectedListing is truthy, even if it has no valid coordinates (e.g. a not-found listing sets { error: true, ... }). In that case the map never applies initialCoordinates (IP fallback) because flyTo is skipped and initialViewState won’t update after mount. Consider changing the guard to only block when hasValidCoordinates(selectedListing) (or when a listing has already been centered), so invalid/errored selections still fall back to initialCoordinates.
| selectedListing || | |
| hasValidCoordinates(selectedListing) || |
cc5400b to
cdc3ba7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cdc3ba7 to
4fc833b
Compare
Summary
latitudeandlongitudecolumns frompublic.listingsand keepslocationas the single stored coordinate source.coordinatesobject for the app.coordinatesand write onlylocation.Why
The flat coordinate columns duplicated the PostGIS geography point and could drift from it. The views now give the frontend a reliable shape without making React parse raw geography/EWKB values.
Validation
npm run supabase:resetpublic.listingsno longer haslatitudeorlongitude, while the listing views exposecoordinates.listings_in_view(...)returns seeded listings with coordinate objects.npm run checknpm run buildwith local Supabase env overridesBrowser screenshot verification was not available locally because
agent-browserwas not installed and the Playwright MCP could not create its root-level runtime directory on the read-only filesystem.